Docker workflow by rbanka1 · Pull Request #1105 · oneapi-src/unified-memory-framework

rbanka1 · 2025-02-18T17:34:57Z

Add Docker workflow
changes:

created docker build and push workflow
created ubuntu 24.04 docker image
modified reuseable_fast and basic to work on docker image
modified pr/push to run build image when needed
added few installation to docker files

Checklist

Code compiles without errors locally
All tests pass locally
CI workflows execute properly

CI workflows, not executed per PR (e.g. Nightly), execute properly
New tests added, especially if they will fail without my changes
Added/extended example(s) to cover this functionality
Extended the README/documentation
All newly added source files have a license
All newly added source files are referenced in CMake files
Logger (with debug/info/... messages) is used

changes: - created docker build and push workflow - created ubuntu 24.04 docker image - modified reuseable_fast and basic to work on docker image - modified pr/push to run build image when needed - added few installation to docker files

PatKamin · 2025-02-26T12:16:24Z

 ARG TEST_DEPS="\
-	libnuma-dev"
+	libnuma-dev \
+	libhwloc-dev \


libhwloc and libtbb are already included in UMF_DEPS - in all Dockerfiles

you have right, I will remove it

heh, nope - you should not remove the UMF_DEPS. These ARGs are not only to list all dependencies, this is also to show which ones are required for using the library, and which are required e.g. for testing.

I believe tbb is not required to compile UMF, but it's required for tests. hwloc is required for UMF, so could be removed from test deps.

Okay that's said - there's another issue that's connected.
libhwloc-dev should be installed only on newer Ubuntu, here on Ubuntu-20.04 we should not install it and only install it via our script.
So in this OS, pls move HWLOC_DEPS up, add no UMF_DEPS, but please make a comment that hwloc is required as UMF dependency.
On other OSes, please remove the hwloc script and HWLOC_DEPS, and use UMF_DEPS equal to libhwloc-dev.

PatKamin · 2025-02-26T12:19:01Z

+  contents: read
+
+jobs: 
+  build-ci-container:


I think this job shall be limited to upstream only:

if: github.repository == 'oneapi-src/unified-memory-framework'

I'm not sure for that. As we were talking, I left it untouched

could you try and add that, pls? I think it should work just fine; at least we should test it.

Of course as long as you're testing you can omit adding it 😉

PatKamin · 2025-02-26T13:04:12Z

+      image: ${{ matrix.image }}
+      options: --user root --privileged
+      volumes:
+        - /home/runner/work/unified-memory-framework/unified-memory-framework:/home/runner/work/unified-memory-framework/unified-memory-framework


Can ${{github.workspace}} be used in case it differs?

yes, i replaced it

PatKamin · 2025-02-26T13:17:34Z

+        ${{matrix.extra_build_options}}
+
+    - name: Build
+      run: cmake --build ${{env.BUILD_DIR}} --config Release -j


-j $(nproc)

PatKamin · 2025-02-26T13:20:21Z

+    strategy:
+      matrix:
+        include:
+          - os: windows-latest


Remove this var as there is only one OS tested

PatKamin · 2025-02-26T13:22:49Z

@@ -91,6 +134,7 @@ jobs:
        -DCMAKE_PREFIX_PATH="${{env.VCPKG_PATH}}"


CMAKE_INSTALL_PREFIX not used, remove

I also left it because it wouldn't work without it https://github.com/rbanka1/unified-memory-framework/actions/runs/13549985319/job/37871871137

lukaszstolarczuk · 2025-02-28T15:58:44Z

 ARG TEST_DEPS="\
-	libnuma-dev"
+	libnuma-dev \
+	libhwloc-dev \


heh, nope - you should not remove the UMF_DEPS. These ARGs are not only to list all dependencies, this is also to show which ones are required for using the library, and which are required e.g. for testing.

I believe tbb is not required to compile UMF, but it's required for tests. hwloc is required for UMF, so could be removed from test deps.

Okay that's said - there's another issue that's connected.
libhwloc-dev should be installed only on newer Ubuntu, here on Ubuntu-20.04 we should not install it and only install it via our script.
So in this OS, pls move HWLOC_DEPS up, add no UMF_DEPS, but please make a comment that hwloc is required as UMF dependency.
On other OSes, please remove the hwloc script and HWLOC_DEPS, and use UMF_DEPS equal to libhwloc-dev.

lukaszstolarczuk · 2025-02-28T17:18:15Z

+          ALL_CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
+        run: |
+          echo "Changed files: ${{ steps.changed-files.outputs.all_changed_files }}"
+  BuildDockers:


pls verify if detectChanges works properly when we merge a PR - pls test scenarios when you merged PR with and without any docker changes

done, test with changes https://github.com/rbanka1/unified-memory-framework/actions/runs/13627636943
test without https://github.com/rbanka1/unified-memory-framework/actions/runs/13627807242/job/38088708419

hmm tbh I'm not sure what has been tested here...? these were not a "test PRs" being merged into main, right? I was thinking - you push your new workflow (with building docker) into your main, then open 2 PRs on your fork and see if they are working properly (on PR). Later, test if after merging they are still working properly.

// for testing purpose I feel like you have to use different actor and set token in secrets

lukaszstolarczuk · 2025-03-03T13:12:26Z

+          ALL_CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
+        run: |
+          echo "Changed files: ${{ steps.changed-files.outputs.all_changed_files }}"
+  BuildDockers:


hmm tbh I'm not sure what has been tested here...? these were not a "test PRs" being merged into main, right? I was thinking - you push your new workflow (with building docker) into your main, then open 2 PRs on your fork and see if they are working properly (on PR). Later, test if after merging they are still working properly.

// for testing purpose I feel like you have to use different actor and set token in secrets

lukaszstolarczuk · 2025-03-03T13:13:58Z

-      run: sudo apt-get install -y ${{matrix.compiler.cxx}}
-
    - name: Install libhwloc
      run: .github/scripts/install_hwloc.sh


this step shouldn't be here - it's already covered in docker

lukaszstolarczuk · 2025-03-03T13:14:27Z

        LD_LIBRARY_PATH="${{env.BUILD_DIR}}/lib/:${LD_LIBRARY_PATH}" ctest --output-on-failure

-    - name: Check coverage
+    - name: Check os coverage


why this change in name?

lukaszstolarczuk · 2025-03-03T13:25:03Z

      working-directory: ${{env.BUILD_DIR}}
      run: |
-        export COVERAGE_FILE_NAME=${{env.COVERAGE_NAME}}-${{matrix.os}}-shared-${{matrix.shared_library}}-no_hwloc-${{matrix.disable_hwloc}}
+        export COVERAGE_FILE_NAME=${{env.COVERAGE_NAME}}-${{matrix.ubuntu_ver}}-shared-${{matrix.shared_library}}-no_hwloc-${{matrix.disable_hwloc}}


ok, that's not good - the name of the artifact will be cov-22.04-shared... - we should definitely keep the old name with "ubuntu" in it - as we may have more OSes than just ubuntu - perhaps you should use the var as before os instead of ubuntu_ver -- so basically turn back what it was in matrix - os: ['ubuntu-20.04', 'ubuntu-22.04']

For unity, the same approach in FastBuilds - os: ['ubuntu-20.04', 'ubuntu-22.04'] instead of ubuntu_ver: <number>

lukaszstolarczuk · 2025-03-03T13:26:53Z

-ARG UMF_DEPS="\
-	libhwloc-dev \
-	libtbb-dev"
+# libhwloc-dev is required


this comment should be a little more verbose - saying that it's installed here via script, as the hwloc version is too old on this OS

and please move HWLOC_DEPS and # Copy hwloc here, above.

lukaszstolarczuk · 2025-03-03T13:32:52Z

      uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
      with:
        fetch-depth: 0



still missing tbb removal for matrix.install_tbb == 'OFF'

lukaszstolarczuk · 2025-05-08T15:20:54Z

continued in #1298

Add Docker workflow

527baa9

changes: - created docker build and push workflow - created ubuntu 24.04 docker image - modified reuseable_fast and basic to work on docker image - modified pr/push to run build image when needed - added few installation to docker files

rbanka1 force-pushed the DockerWorkflow branch from d1700d4 to 527baa9 Compare February 18, 2025 17:39

lukaszstolarczuk mentioned this pull request Feb 19, 2025

[WIP] Docker in the nightly testing #1033

Closed

10 tasks

lukaszstolarczuk reviewed Feb 19, 2025

View reviewed changes

corrections

25d9501

lukaszstolarczuk reviewed Feb 25, 2025

View reviewed changes

PatKamin reviewed Feb 26, 2025

View reviewed changes

rbanka1 added 2 commits February 28, 2025 08:26

corrections v2

3c7360d

add GH user

c854942

PatKamin reviewed Feb 28, 2025

View reviewed changes

Comment thread .github/workflows/reusable_fast.yml

Comment thread .github/workflows/reusable_fast.yml

corrections V3

c26684e

lukaszstolarczuk reviewed Feb 28, 2025

View reviewed changes

Comment thread .github/workflows/reusable_basic.yml Outdated

lukaszstolarczuk reviewed Feb 28, 2025

View reviewed changes

correctionsV4

eece86b

lukaszstolarczuk reviewed Mar 3, 2025

View reviewed changes

lukaszstolarczuk closed this May 8, 2025

rbanka1 deleted the DockerWorkflow branch May 22, 2025 13:04

		@@ -91,6 +134,7 @@ jobs:
		-DCMAKE_PREFIX_PATH="${{env.VCPKG_PATH}}"

Conversation

rbanka1 commented Feb 18, 2025

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!